-
Notifications
You must be signed in to change notification settings - Fork 163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
support editing connection with unmatched connection type #3326
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3326 +/- ##
==========================================
- Coverage 84.85% 84.83% -0.02%
==========================================
Files 1321 1321
Lines 29446 29471 +25
Branches 8037 8049 +12
==========================================
+ Hits 24986 25002 +16
- Misses 4460 4469 +9
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
@@ -160,15 +158,15 @@ export const ManageConnectionModal: React.FC<Props> = ({ | |||
}); | |||
}} | |||
error={error} | |||
isSubmitDisabled={!isFormValid || !isModified} | |||
isSubmitDisabled={!isFormValid || !isModified || !connectionTypeName || isSaving} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit pick, || !connectionTypeName
is duplicated logic that is already done in !isFormValid
I believe
const [connectionTypes, connectionTypesLoaded, connectionTypesError] = | ||
useWatchConnectionTypes(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the <ManageConnectionModal />
will filter out disabled connections from being selectable, unless it's isEdit
via enabledConnectionTypes
.
<ConnectionTypeForm
connectionTypes={isEdit ? connectionTypes : enabledConnectionTypes}
The current behavior in the main branch will pull up the correct field types of a disabled connection type. This will make it act like a missing lookup. If we want to keep the current behavior, the useWatchConnectionTypes
doesn't need to be modified.
If we go with the new changes, you should probably remove the references to enabledConnectionTypes
in frontend/src/pages/projects/screens/detail/connections/ManageConnectionsModal.tsx
. But I don't think we should because the notebook connections modal's parent will also need to be updated. keeping this logic inside the ManageConnectionsModal.tsx
encapsulate this logic.
Or we want the new behaviour you can change in the modal
<ConnectionTypeForm
connectionTypes={enabledConnectionTypes}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought out this when I made the change. However I'm ok changing this back so that we can edit connections who's types have since been disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the final verdict to change it back? if so it's still failing the lookup from line 179
options={!isEdit ? enabledConnectionTypes : undefined}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emilys314 this is working for me as well as one of the unit tests.
I have an s3
connection type which is disabled. I then create a data connection and then edit it from the connections tab. The dropdown is disabled and shows the correct type in the dropdown. Similarly it in the table it also shows the correct type name.
As for line 179. When editing, I made it so that we do not supply options which will result in a disabled dropdown and it will create the single option based on the connection type passed it for editing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh okay sorry, looks like my cluster didn't update or something, it's working now
@@ -31,6 +31,8 @@ const getConnectionTypeSelectOptions = ( | |||
isPreview: boolean, | |||
selectedConnectionType?: ConnectionTypeConfigMapObj, | |||
connectionTypes?: ConnectionTypeConfigMapObj[], | |||
disableTypeSelection?: boolean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit pick. Imo this isn't an accurate name for doing logic. Because in the preview form we also disable the type selection. A more accurate boolean would say the existing type reference was missing. (Also i could be wrong, but I don't think this boolean is needed at all for this logic?)
60aea6c
to
e378a3a
Compare
[connectionTypes], | ||
); | ||
|
||
const connectionTypeRef = connection?.metadata.annotations['opendatahub.io/connection-type']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this a ref before? should the name be updated?
Other than that, everything look good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was not a react ref. I have renamed it to not be confused with React ref.
e378a3a
to
5680d01
Compare
/retest |
5680d01
to
5cb948c
Compare
rebased to re-run checks |
5cb948c
to
12ebf0a
Compare
/retest |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
https://issues.redhat.com/browse/RHOAIENG-14274
https://issues.redhat.com/browse/RHOAIENG-14273
Description
Updates the empty state
Add connection
button to open the modal. Also disables theAdd connection
buttons when no connection types are present (note that this should not happen when we install OOTB types).The annotation
opendatahub.io/connection-type
value from theSecret
is used to reference a connection type. When no matching connection type is found now this reference is displayed as the connection type in the disabled drop down and all fields are displayed in their default state (env vars as labels and fields are all text inputs).How Has This Been Tested?
disableConnectionsTypes
feature flag tofalse
.Data connections
tab of the project details.Connections
tab and edit the data connection.s3
.s3
connection type (until we have OOTB types) and use the admin page. It can be enabled or disabled (test both).S3 connection type
Connections
tab and edit the same connection.S3 compatible object storage - v1
. This time the connection type was used to generate the form.Test Impact
Add unit test.
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main
cc @simrandhaliw